-
Notifications
You must be signed in to change notification settings - Fork 77
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add capture_(request|response)_headers
to instrument_httpx
#671
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #671 +/- ##
==========================================
Coverage 100.00% 100.00%
==========================================
Files 137 137
Lines 10721 10835 +114
Branches 1470 1488 +18
==========================================
+ Hits 10721 10835 +114 ☔ View full report in Codecov by Sentry. |
Deploying logfire-docs with Cloudflare Pages
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Need to handle and test passing a sync hook for an async client when headers aren't being captured
async_request_hook = cast('AsyncRequestHook | None', final_kwargs.get('async_request_hook')) | ||
async_response_hook = cast('AsyncResponseHook | None', final_kwargs.get('async_response_hook')) | ||
|
||
if capture_request_headers: # pragma: no cover |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
non-blocking, but things here are getting complicated enough that this is worth testing. it could either make a real request or a mini local server could be set up in another thread.
client: None, | ||
capture_request_headers: bool = False, | ||
capture_response_headers: bool = False, | ||
**kwargs: Unpack[HTTPXInstrumentKwargs], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Still want to get rid of these Unpacks and kwargs eventually.
No description provided.